Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

218 Fixed failed pipeline #221

Merged
merged 2 commits into from
Aug 29, 2024
Merged

218 Fixed failed pipeline #221

merged 2 commits into from
Aug 29, 2024

Conversation

donyunardi
Copy link
Contributor

Fixes #218

image

@donyunardi donyunardi linked an issue Aug 23, 2024 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Aug 23, 2024

✅ All contributors have signed the CLA
Posted by the CLA Assistant Lite bot.

@donyunardi
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@donyunardi donyunardi enabled auto-merge (squash) August 23, 2024 03:38
Copy link
Contributor

github-actions bot commented Aug 23, 2024

badge

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ------------------------------------------------------------------------------
R/all_choices.R                       1       0  100.00%
R/call_utils.R                      156     124  20.51%   19-28, 65, 67, 69, 102-345
R/check_selector.R                   33       0  100.00%
R/choices_labeled.R                 153      27  82.35%   68, 74, 79, 86, 102, 221-225, 229-234, 354-355, 357, 363, 390-397
R/choices_selected.R                 81      11  86.42%   210-238, 275
R/column_functions.R                  3       3  0.00%    15-18
R/data_extract_datanames.R           30       8  73.33%   16-20, 83-85
R/data_extract_filter_module.R      102      47  53.92%   91-104, 106-107, 109-126, 142-161
R/data_extract_module.R             298      67  77.52%   128, 133, 150, 153-158, 160, 179-182, 212-258, 497, 502, 679, 690-691, 769-774
R/data_extract_read_module.R        137       7  94.89%   34, 39-41, 43, 138, 155
R/data_extract_select_module.R       32      18  43.75%   29-46
R/data_extract_single_module.R       60       2  96.67%   30, 43
R/data_extract_spec.R                32       0  100.00%
R/filter_spec.R                     186       1  99.46%   280
R/format_data_extract.R              16       1  93.75%   48
R/get_dplyr_call.R                  297       0  100.00%
R/get_merge_call.R                  278      29  89.57%   32-38, 49, 215-224, 391, 407-419
R/include_css_js.R                    5       0  100.00%
R/input_checks.R                     11       2  81.82%   17-18
R/merge_data_utils.R                  2       0  100.00%
R/merge_datasets.R                  134       6  95.52%   123, 249-253
R/merge_expression_module.R          60      11  81.67%   161-166, 184, 362-367
R/Queue.R                            23       0  100.00%
R/resolve_delayed.R                  16       4  75.00%   78-81
R/resolve.R                         113      44  61.06%   179-285
R/select_spec.R                      64       8  87.50%   99, 179-186
R/utils.R                            37      24  35.14%   33-46, 174-187
R/zzz.R                               3       3  0.00%    2-4
TOTAL                              2363     447  81.08%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 019e3e0

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Aug 23, 2024

Unit Tests Summary

  1 files   24 suites   6s ⏱️
189 tests 189 ✅ 0 💤 0 ❌
659 runs  659 ✅ 0 💤 0 ❌

Results for commit 019e3e0.

♻️ This comment has been updated with latest results.

@donyunardi
Copy link
Contributor Author

Docs workflow is failing and I am able to reproduce it locally after installing the latest evaluate package

Here is the traces:

r$> .Last.error
<callr_error/rlib_error_3_0/rlib_error/error>
Error: 
! in callr subprocess.
Caused by error in `build_reference()`:
! Failed to parse Rd in data_extract_multiple_srv.Rd
Caused by error in `is_call(x) && !is_call(x, "=") && !is_syntactic(x[[1]])`:
! 'length = 3' in coercion to 'logical(1)'See `$stderr` for standard error.
---
Backtrace:
1. pkgdown::build_site()
2. pkgdown:::build_site_external(pkg = pkg, examples = examples, run_dont_run = run_dont_run, …
3. callr::r(function(..., cli_colors, hyperlinks, pkgdown_internet) { …
4. callr:::get_result(output = out, options)
5. callr:::throw(callr_remote_error(remerr, output), parent = fix_msg(remerr[[3]]))
---
Subprocess backtrace:
 1. pkgdown::build_site(...)
 2. pkgdown:::build_site_local(pkg = pkg, examples = examples, run_dont_run = run_dont_run, …
 3. pkgdown::build_reference(pkg, lazy = lazy, examples = examples, run_dont_run = run_dont_run, …
 4. pkgdown:::unwrap_purrr_error(purrr::map(topics, build_reference_topic, …
 5. base::withCallingHandlers(code, purrr_error_indexed = function(err) { …
 6. purrr::map(topics, build_reference_topic, pkg = pkg, lazy = lazy, …
 7. purrr:::map_("list", .x, .f, ..., .progress = .progress)
 8. purrr:::with_indexed_errors(i = i, names = names, error_call = .purrr_error_call, …
 9. base::withCallingHandlers(expr, error = function(cnd) { …
10. purrr:::call_with_cleanup(map_impl, environment(), .type, .progress, …
11. local .f(.x[[i]], ...)
12. base::withCallingHandlers(data_reference_topic(topic, pkg, examples_env = examples_env, …
13. pkgdown:::data_reference_topic(topic, pkg, examples_env = examples_env, …
14. pkgdown:::as_data(tags$tag_usage[[1]])
15. pkgdown:::as_data.tag_usage(tags$tag_usage[[1]])
16. base::vapply(parsed, needs_tweak, logical(1))
17. local FUN(X[[i]], ...)
18. base::.handleSimpleError(function (err) …
19. local h(simpleError(msg, call))
20. cli::cli_abort("Failed to parse Rd in {.file {topic$file_in}}", …
21. | rlang::abort(message, ..., call = call, use_cli_format = TRUE, …
22. | rlang:::signal_abort(cnd, .file)
23. | base::signalCondition(cnd)
24. (function (cnd) …
25. cli::cli_abort(message, location = i, name = name, parent = cnd, …
26. | rlang::abort(message, ..., call = call, use_cli_format = TRUE, …
27. | rlang:::signal_abort(cnd, .file)
28. | base::signalCondition(cnd)
29. (function (err) …
30. rlang::cnd_signal(err$parent)
31. rlang:::signal_abort(cnd)
32. base::signalCondition(cnd)
33. global (function (e) …

@insightsengineering/nest-core-dev any suggestion on what's going on?

SessionInfo()
r$> sessionInfo()
R version 4.4.1 (2024-06-14)
Platform: x86_64-apple-darwin20
Running under: macOS Ventura 13.6.7

Matrix products: default
BLAS:   /System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/Versions/A/libBLAS.dylib 
LAPACK: /Library/Frameworks/R.framework/Versions/4.4-x86_64/Resources/lib/libRlapack.dylib;  LAPACK version 3.12.0

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

time zone: America/Los_Angeles
tzcode source: internal

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

loaded via a namespace (and not attached):
 [1] vctrs_0.6.5             cli_3.6.2               knitr_1.46              rlang_1.1.3             xfun_0.43              
 [6] processx_3.8.4          purrr_1.0.2             glue_1.7.0              htmltools_0.5.8.1       ps_1.7.6               
[11] fansi_1.0.6             rmarkdown_2.28          evaluate_0.24.0         tibble_3.2.1            fastmap_1.1.1          
[16] yaml_2.3.8              lifecycle_1.0.4         compiler_4.4.1          fs_1.6.4                nesttemplate_0.0.0.9008
[21] pkgconfig_2.0.3         digest_0.6.35           R6_2.5.1                utf8_1.2.4              pillar_1.9.0           
[26] callr_3.7.6             magrittr_2.0.3          tools_4.4.1             withr_3.0.1             pkgdown_2.1.0          
[31] desc_1.4.3

@donyunardi
Copy link
Contributor Author

donyunardi commented Aug 23, 2024

Found the problem, which is on the inline expression in argument like this:

data_extract_multiple_srv.list <- function(data_extract,
                                           datasets,
                                           join_keys = NULL,
                                           select_validation_rule = NULL,
                                           filter_validation_rule = NULL,
                                           dataset_validation_rule = if (
                                             is.null(select_validation_rule) &&
                                               is.null(filter_validation_rule)
                                           ) {
                                             NULL
                                           } else {
                                             shinyvalidate::sv_required("Please select a dataset")
                                           },
                                           ...) {

Which we have here and here.

If I temporarily set dataset_validation_rule = NULL, I'm able to build the docs/site locally.
Maybe there are some changes on the newer version of pkgdown.

@donyunardi
Copy link
Contributor Author

@gogonzo

Would it be a breaking change if I set dataset_validation_rule = NULL and move the logic to the function body?

data_extract_multiple_srv.list <- function(data_extract,
                                           datasets,
                                           join_keys = NULL,
                                           select_validation_rule = NULL,
                                           filter_validation_rule = NULL,
                                           dataset_validation_rule = NULL, # setting it to NULL
                                           ...) {
  # move the logic to function's body
  if (is.null(dataset_validation_rule)) {
    if (!is.null(select_validation_rule) || !is.null(filter_validation_rule)) {
      dataset_validation_rule <- shinyvalidate::sv_required("Please select a dataset")
    }
  }

@gogonzo
Copy link
Contributor

gogonzo commented Aug 26, 2024

Would it be a breaking change if I set dataset_validation_rule = NULL and move the logic to the function body?

No, unless somebody specified dataset_validation_rule = NULL somewhere already for some reason. I have completely no clue why is.null() in the condition doesn't work, but I've checked several solutions and this seems to work:

dataset_validation_rule = if (
                                    length(select_validation_rule) || 
                                    length(filter_validation_rule)
                                  ) {
                                    shinyvalidate::sv_required("Please select a dataset")
                                  }

@pawelru
Copy link
Contributor

pawelru commented Aug 26, 2024

That sounds familiar. Take a look at this: r-lib/pkgdown#2727
Issue had been raised and resolved already. It's just not yet released.
To make it pass, I would recommend to use extra-deps workflow parameter (an example in tern here). Please just make sure to annotate that it's temporary and link it to the issue / PR so that we can revert it once released without much investigation.

FYI: We have already done this for our docs workflow but not in the scheduled

@donyunardi
Copy link
Contributor Author

donyunardi commented Aug 26, 2024

FYI: We have already done this for our docs workflow but not in the scheduled

The docs failure is not from the Scheduled workflow, but from the Docs workflow that ran against this branch:
https://github.com/insightsengineering/teal.transform/actions/runs/10519469674/job/29147117999#step:12:175

image

Scheduled workflow is okay:
image

Do you think I can still use the solution that we did in tern for Docs workflow?
if so, where should I add the extra-deps parameter?

@pawelru
Copy link
Contributor

pawelru commented Aug 27, 2024

Oh sorry I was thinking you are referring to the scheduled one. I'm afraid that for the "docs" workflow there is no such parameter. There would be one in the future once we complete phase out of staged deps but this is not there yet.

Let me ask @cicdguy for help. We chatted some time ago about this issue. Was it done? If not - what would be the best way to install dev version of pkgdown? Include in the image or install on the fly?

@m7pr
Copy link
Contributor

m7pr commented Aug 28, 2024

I restarted the check to see if this is Docs failure is temporary or not https://github.com/insightsengineering/teal.transform/actions/runs/10591947450

@m7pr
Copy link
Contributor

m7pr commented Aug 28, 2024

Hey @donyunardi just checking if the addition of rmarkdown to VignetteBuilder slot is the intentional change in here.
rmarkdown engine is already included in knitr so there is no need to specify rmarkdown next to knitr in VignetteBuilder.
Have a look at tidyr package where they have the same scope of rmarkdown vignettes and knitr in DESCRIPTION https://github.com/tidyverse/tidyr/

@pawelru
Copy link
Contributor

pawelru commented Aug 28, 2024

Hey @m7pr please have a look here for resources why.

@donyunardi donyunardi merged commit 2494138 into main Aug 29, 2024
26 of 27 checks passed
@donyunardi donyunardi deleted the 218-fix-failed-pipeline branch August 29, 2024 14:45
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix failed pipeline
4 participants